Skip to content

fix(auth): coordinate token refresh across pages - #3377

Merged
aofei merged 12 commits into
goplus:devfrom
nighca:issue-3376
Jul 31, 2026
Merged

fix(auth): coordinate token refresh across pages#3377
aofei merged 12 commits into
goplus:devfrom
nighca:issue-3376

Conversation

@nighca

@nighca nighca commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Closes #3376

Changes

  • serialize access-token retrieval across browser pages with the Web Locks API
  • reload shared credentials after acquiring the lock so waiting pages reuse the refreshed token
  • synchronize user state through localStorage

Verification

  • pnpm exec vitest --run src/stores/user/signed-in.test.ts
  • pnpm run type-check
  • pnpm run lint -- src/stores/user/signed-in.ts src/stores/user/signed-in.test.ts

@nighca

nighca commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

@xgopilot /review

@fennoai

fennoai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review complete

Posted a non-blocking review with 6 inline comments and a summary: pullrequestreview-4824626550.

Highlights

  • ✅ Sound core: navigator.locks serializes refresh across pages, and re-reading credentials inside the lock lets a waiting page reuse another page's freshly refreshed token.
  • Cross-tab echo writes: a storage event → restoreUserState() → reactive mutation → watchEffect re-writes the same value in every listening tab (bounded, not infinite, but N redundant setItems).
  • initUserState registers a storage listener + watchEffect with no cleanup or double-init guard.
  • The storage listener now adopts unvalidated localStorage state live across tabs (the Object.assign(JSON.parse(...)) itself is pre-existing; live adoption is new) — consider validating the parsed shape.
  • Persisting the refreshToken to localStorage is pre-existing on dev, not introduced by this PR — noted as context only.
  • Lock is held across a second network call (username fetch) that doesn't need the critical section.
  • Tests stub navigator.locks and don't exercise real contention or the watchEffectstorage interaction.

All findings are non-blocking suggestions.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: coordinate token refresh across pages

The core approach is sound — serializing refresh via navigator.locks and re-reading shared credentials inside the lock (restoreUserState() before re-checking isAccessTokenValid()) is exactly the right pattern so a waiting page reuses a token another page just refreshed. Capturing refreshTokenBeforeRefresh before the async call and the double-check inside the lock are nicely done, and the test confirms a waiting page reuses another page's fresh token without calling refreshToken.

A few points worth considering before merge (all non-blocking):

Context (pre-existing, not introduced here): Persisting the full auth state — including the long-lived refreshToken — to localStorage, and adopting it via unvalidated Object.assign(userState, JSON.parse(stored)), already exists on dev. This PR does not newly introduce token persistence. What is new is that the storage listener now makes another tab's localStorage contents adopted live at runtime, which widens that unvalidated-adoption surface (see inline note).

Tests: The concurrency test stubs navigator.locks.request and never exercises two overlapping ensureAccessToken() calls contending for a real lock, so serialization / single-flight is asserted only indirectly. There is also no test for the watchEffectstorage-listener interaction (the highest-risk new behavior). Consider adding a contention test (two overlapping calls, refreshToken called once) and a test that an incoming storage update does not trigger a redundant echo write.

See inline comments for the specific spots.

Comment thread spx-gui/src/stores/user/signed-in.ts Outdated
Comment thread spx-gui/src/stores/user/signed-in.ts
Comment thread spx-gui/src/stores/user/signed-in.ts Outdated
Comment thread spx-gui/src/stores/user/signed-in.ts Outdated
Comment thread spx-gui/src/stores/user/signed-in.ts
Comment thread spx-gui/src/stores/user/signed-in.ts
@aofei
aofei merged commit b6056ef into goplus:dev Jul 31, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User loses login state after leaving multiple Builder pages open

2 participants